Self as SubSequence was an anti-pattern. That violated "The subsequence shares indices with the original collection".

Probably we should not read the memory before the lock, or we should use atomic read/write. But does it really matter?

Yeah I think you either need to guard this with the lock, or use atomics. Another thread could potentially perform the writes out-of-order, so the buffer may not be fully initialized. SE-0282 requires concurrent read/write accesses to be done using atomic operations (a lock also works as it would make them non-concurrent)

Thanks for the reference! Changed to use _Atomic(const void *), and it doesn't seem to affect the performance numbers.

Nice! Could be worth seeing if this can be done purely using atomics to avoid needing PlatformMutex, e.g compare-and-exchange a null pointer with 0x1 on the thread that computes the layout, with racing threads spinning until it becomes > 0x1.

Edit: Having thought about this a bit more, I'm not sure it's actually a good idea: we'd still need a mutex to guard the allocator, and doing a spinlock can potentially lead to deadlocks

I thought about this a bit and think that we should be a little more conservative with the memory that we allocate. My rationale is as follows (if you agree, I make this a doc comment).

We can’t know the size required to hold all SyntaxData for the entire tree until we have traversed it because totalNodes only counts the non-nil syntax nodes in the tree (which require the size of SyntaxData plus the size of AtomicPointer by which the SyntaxData is referenced from the parent node). Additionally, all layout children that are nil require the size of AtomicPointer but these nil-slots are not accounted for in totalNodes, so we need to estimate the ratio of nil children to non-nil children. The facts that we can base our estimate on are:

• In a valid syntax tree, every layout node with n children has n + 1 unexpected* children, which are nil for valid syntax trees, which account for the majority of syntax trees. We can thus expect at least 1 nil-child for every non-nil child.
• Some layout nodes have optional children, which further increases the number of nil-children.
• Collection nodes don’t have nil children, which offsets the optional children to some degree.

Based on that information, we can choose to over- or underestimate:

• Under-estimating the slab size by a constant-ish factor of 2 or 3 is not a big deal because it means that we just need to allocate another 1 or 2 slabs if the user does a full-tree traversal. If the user doesn’t traverse the entire tree, we save a little memory.
• Over-estimating the slab size if the full tree size is less than 4096 means that we allocate memory which we’ll never use.

We thus assume that every non-nil child has roughly one child that is nil.

I want to keep single node trees minimal. Note that single node trees are often created when rewriting TokenSyntax in SyntaxRewriter. MemoryLayout<SyntaxData>.stride (32 bytes) is enough for them. I don't want to waste extra pointers size. So I think we should handle root node specially.

As for the actual estimation, actually measured with our swift-syntax test suite:

• with my estimation logic, 1,043/998,482 (0.1%) trees created with slabSize < 4096 overflows the estimated slab size,
• with your estimation logic, 54,601/1,047,632 (5.2%) trees created with slabSize < 4096 overflowed
• fwiw, (dataSize + pointerSize * 3) * nodeCount still overflows in 28,417/1,020,774 trees.

And I think under-estimating the slab size is more problematic than over-estimating. because it wastes most of the second slab.

I didn’t think about the single-node trees. That’s a good point and a worthwhile optimization. Maybe worth a comment.

Your analysis is correct under the assumption that the user visits the entire tree, which might not necessarily be true. And it might be worth measuring how much memory in the slabs are not used with the different estimation logics.

Why is the denominator of your measurements different for the different estimation logics? I would expect them to be the same because you should have created the same number of trees by running the test suite.

Your analysis is correct under the assumption that the user visits the entire tree, which might not necessarily be true.

FWIW my measurements are based on the actual test cases we have. They might be different from the typical use-cases, but I just wanted to point out that.

It's true that users don't always visit the entire tree, but I don't think some overallocation is that problematic. (of course we want to avoid allocating memory that we know will never be used)

And it might be worth measuring how much memory in the slabs are not used with the different estimation logics.

That would be interesting 👍

Why is the denominator of your measurements different for the different estimation logics? I would expect them to be the same because you should have created the same number of trees by running the test suite.

The denominator is the number of "trees created with slabSize < 4096" which is different between the estimation logics.

We will revisit this.

Not related to this PR, but out of curiosity, is there any reason rootId isn't RawSyntax.ID?

Maybe we thought it should be serializable. But at this point, I don't think there's a reason. Actually I was thinking to make it just RawSyntax.ID in a follow up :)

Now that you mention serializing, that might have been the reason when we still transferred the syntax tree via XPC or JSON from the C++ parser in 2018-ish.